Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set session timezone to local timezone on startup #1482

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

maximsmol
Copy link

Description

Sets the session time zone to the system time zone after connecting.

Adds tzlocal as a dependency for a cross-platform way of getting the current time and handling the TZ environment variable. I've considered including code for this directly but we need to handle a lot of different Windows quirks. This package adds 126KB to the on-disk installation size.

Checklist

  • I've added this contribution to the changelog.rst.
  • I've added my name to the AUTHORS file (or it's already there).
  • I installed pre-commit hooks (pip install pre-commit && pre-commit install), and ran black on my code.

Signed-off-by: maximsmol <[email protected]>
Signed-off-by: maximsmol <[email protected]>
Signed-off-by: maximsmol <[email protected]>
@dbaty
Copy link
Member

dbaty commented Oct 17, 2024

Thanks for your contribution! :) I am not familiar with the set time zone command (though I have read its doc). I feel like it could lead to unexpected results (and a difference in behaviour with what we have now in pgcli, and also a difference between pgcli and psql).

For example, suppose that the server is on timezone "tz0", user1 on tz1, user2 on tz2, user3 on tz3. Currently, all users calling now() will get the same timestamp with time zone tz0 (the server timezone). With your proposal, they could get 3 different things:

  • user1 uses pgli and gets a timestamp on timezone tz1
  • user2 uses pgli and gets a timestamp on timezone tz2
  • user3 uses psql (not pgcli) and gets a timestamp on timezone tz0 (server timezone).

We end up with 3 different values. They represent the same time, only expressed under 3 different timezones, but still these are different values. Also, what concerns me is that what you get in pgcli is different from what you get on psql. That in itself seems problematic to me. At least, problematic enough that this feature should not be activated by default.

Could you please comment on that and clarify the other changes in behaviour (if any)?

I believe that this kind of "customization" should rather take place in "startup commands". The idea of such a feature has been expressed some time ago (see #247). What do you think?

@j-bennet : what's your take on this?

@maximsmol
Copy link
Author

maximsmol commented Oct 17, 2024

Thanks for responding so quickly.

Concerns

To summarize, there are two concerns:

  1. different users may see the same value in time represented by visually different values,
  2. behavior is different from psql.

In response:

  1. Note that the values are in fact equal according to Postgres itself, despite having different string representations and time zone offsets. This is also the case in other popular languages (see below for experiments).
  2. Being different from psql is, for me, the entire point of using pgcli.

The only other potential issue I can come up with is for people using timestamp without timezone since they would actually resolve to different moments in time as they are always in the "current" time zone. Those are already frowned upon for a lot of other reasons, however.

Startup Commands

I think "startup commands" would be nice in addition to this PR. It's not a good replacement because to fully replicate this behavior you would need to set the commands dynamically each time, in case the time zone changed (laptops in particular tend to move time zones a lot).

Here is what we currently do to get psql (which does support running commands before a REPL) to use the local time zone:

#!/usr/bin/env bash

tz_data_raw=$(strings /etc/localtime| tail -1)
IFS=',' read -r -a tz_data <<< "$tz_data_raw"

URL="postgresql://$SQL_ADMIN_USER:$SQL_ADMIN_PW@$SQL_DB_HOST:$SQL_DB_PORT/$SQL_DB_NAME$SQL_URL_EXTRA"
psql "$URL" -c "set time zone ${tz_data[0]}" "$@"

As you can see, it's not a great solution.


As a compromise, maybe we can make this opt-in with a config parameter? I am generally against random configuration parameters but if there is an impasse I'd rather do that than have to make each member of my team install a fork.

Thoughts

  • Showing time in the current user's local time is a ubiquitous default in interactive programs. Even calendar programs do this despite much larger potential issues (e.g. they are the only place where timezone-naive datetime can be preferable).
  • Converting from UTC in places with daylight savings rules, large offsets is really annoying, especially for queries that span months or years.
  • Converting from non-UTC time zones for queries that include international data is even worse. Realized this is false because they will all show up in UTC (or whatever the server default time zone is)
  • UTC timestamps sometimes cause confusion if users do not consciously make the conversion.
  • This is one of the few parts where the experience is currently worse than in psql where it is at least technically possible to automatically set the current time zone. This could alternatively be solved with startup commands, though I doubt people would use the ugly script above and so would likely have to manually adjust the time zone command periodically.

Experiment: Are Datetimes In Different Time Zones Equal?

Postgres:

> select '2024-10-17 00:00:00-07'::timestamptz = '2024-10-17 01:00:00-06'::timestamptz
?column?
True
SELECT 1
Time: 0.040s

JavaScript:

> const x = new Date("2024-10-17 00:00:00-07")
> const y = new Date("2024-10-17 01:00:00-06")
> x - y === 0
true
> x >= y && x <= y
true
> x === new Date("2024-10-17 00:00:00-07")
// this is to show that `===` is referential equality here for some reason
false

Python:

>>> x
datetime.datetime(2024, 10, 16, 0, 0, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=61200), 'PDT'))
>>> x.astimezone(timezone(timedelta(0)))
datetime.datetime(2024, 10, 16, 7, 0, tzinfo=datetime.timezone.utc)
>>> x == x.astimezone(timezone(timedelta(0)))
True

pgcli/main.py Outdated
@@ -1624,6 +1625,8 @@ def cli(
else:
pgcli.connect(database, host, user, port)

pgcli.pgexecute.set_timezone(tzlocal.get_localzone_name())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this ever raise an exception or return a None?
What happens if I set TZ to a non-existent timezone name?

Copy link
Author

@maximsmol maximsmol Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • It does not throw if TZ is an invalid value (I also tested this directly)
  • It would throw if multiple conflicting timezone configurations are found in one of the above files. I added a try-catch to print a warning and continue without setting a custom timezone (using the default server settings instead)
  • It might return None if no timezone configuration is found. I added a check that prints a warning and continues without setting a timezone

For due diligence, from reading the source code:

  • It will throw if TZ points to a file (absolute path) and os.path.exists or realpath return throw an error
  • It could throw if exists("/system/bin/getprop") throws
  • It could throw if exists, islink or realpath throws for /etc/localtime
  • It could throw if realpath throws for a timezone config referred to from /etc/timezone, /var/db/zoneinfo, /etc/sysconfig/clock, /etc/conf.d/clock, or /etc/localtime

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants